Skip to content

Conversation

@brkyvz
Copy link
Contributor

@brkyvz brkyvz commented Jul 13, 2016

What changes were proposed in this pull request?

It's unnecessary. QueryTest already sets it.

@brkyvz
Copy link
Contributor Author

brkyvz commented Jul 13, 2016

cc @marmbrus

@SparkQA
Copy link

SparkQA commented Jul 13, 2016

Test build #62206 has finished for PR 14170 at commit 01dbc42.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

LGTM, can you make a JIRA? Its a little scary to change tests w/o one in case there is flakiness.

@marmbrus
Copy link
Contributor

We should put this in 2.0 for whoever merges.

@brkyvz brkyvz changed the title [MINOR][SQL][TEST] Remove timezone setting from DataFrameTimeWindowingSuite [SPARK-16531][SQL][TEST] Remove timezone setting from DataFrameTimeWindowingSuite Jul 13, 2016
@brkyvz
Copy link
Contributor Author

brkyvz commented Jul 13, 2016

Created JIRA SPARK-16531

@marmbrus
Copy link
Contributor

Thanks, merging to master and 2.0

asfgit pushed a commit that referenced this pull request Jul 13, 2016
…ndowingSuite

## What changes were proposed in this pull request?

It's unnecessary. `QueryTest` already sets it.

Author: Burak Yavuz <brkyvz@gmail.com>

Closes #14170 from brkyvz/test-tz.

(cherry picked from commit 0744d84)
Signed-off-by: Michael Armbrust <michael@databricks.com>
@srowen
Copy link
Member

srowen commented Jul 13, 2016

It may be a secondary point, but QueryTest should reset the timezone like the code being removed does. Right now it means that the timezone default stays changed for any other tests in the JVM.

@asfgit asfgit closed this in 0744d84 Jul 13, 2016
@marmbrus
Copy link
Contributor

marmbrus commented Jul 13, 2016

All the tests in SQL are written to assume Los_Angeles, so I think this is actually desired. Otherwise people have to configure their machine specially to run spark tests. The issue here was it was UTC, which I think was causing some flakyness when tests are run in parallel.

@srowen
Copy link
Member

srowen commented Jul 13, 2016

They should extend a test that explicitly sets that if so, or else it only happens to be set because of test ordering. The issue really is that it gets set for all remaining tests not in SQL. That could be OK; set to something fixed is better than defaulting to the local value. But then even that depends on whether SQL tests ran, and when and in what JVM.

@marmbrus
Copy link
Contributor

I think thats where we are today. All query tests use LA and the harness configures that. The problem before this PR was this one suite was setting LA (due to its base class), and then UTC (due to its own before/after all), and that was messing with other tests that assume LA.

@srowen
Copy link
Member

srowen commented Jul 14, 2016

Yes, I got that, the problem is (potentially) in the non-SQL tests. If nothing's failing today, probably is entirely fine in practice, but this could have been made more robust while we're here.

@marmbrus
Copy link
Contributor

marmbrus commented Jul 14, 2016

Oh, sorry, I see what you are saying, although I'm not sure I agree with the conclusion. Given that tests can run in parallel I don't think you actually want to toggle back and forth between timezones. I actually think that was causing the flakeyness we are trying to fix.

In contrast, setting the default timezone to something fixed across all suites in Spark is idempotent. So, if any suite that cares about the default timezone just sets it to LA you should be fine even in the face of concurrency.

@srowen
Copy link
Member

srowen commented Jul 14, 2016

Agree. I'd rather fix a timezone for all tests. Some tests don't happen to set one; I know some use GMT elsewhere. Well, it's an issue for another day, to standardize it if it causes enough pain at some point.

@brkyvz brkyvz deleted the test-tz branch February 3, 2019 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants